fix: preserve inner task names and raise on unknown task in EnvGroup#1216
fix: preserve inner task names and raise on unknown task in EnvGroup#1216tashapais wants to merge 3 commits intoPrimeIntellect-ai:mainfrom
Conversation
When an EnvGroup is wrapped inside another EnvGroup, the outer group was unconditionally overwriting the inner task names with a single outer name, then silently falling back to envs[0] when routing failed. Two fixes: - When a sub-env is itself an EnvGroup, register each of its task names in the outer env_map pointing to the inner group, and keep the inner task column as-is so routing works through both levels. - get_env_for_task now raises ValueError on unknown tasks instead of silently misrouting to envs[0], making misconfiguration immediately visible. Fixes PrimeIntellect-ai#1008 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
Fixes #1008. Two changes: (1) when a sub-env is itself an EnvGroup, its inner task names are now registered in the outer env_map (pointing to the inner group) rather than being overwritten with the outer name -- routing then flows correctly through both levels; (2) get_env_for_task raises ValueError on unknown tasks instead of silently falling back to envs[0]. |
Three issues found in review: 1. Name collision: inner task names from a nested EnvGroup now raise ValueError at construction if they conflict with a sibling env's name, rather than silently overwriting the sibling's env_map entry. 2. Stale outer name: the outer name (e.g. "my_env") is now removed from env_map after its inner task names are expanded, so get_env_for_task on the outer name raises cleanly instead of routing to the inner group which would then also raise for the same unknown name. 3. Docs: update docs/environments.md to document the ValueError on unknown tasks, nested EnvGroup support, and the name-uniqueness requirement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed all three Bugbot findings in the latest commit (cebd14c):
@willccbb @mikasenghaas happy to make any adjustments. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cebd14c. Configure here.
Three issues from bugbot review: 1. The unconditional pop(name) removed a mapping just registered when the outer name happened to equal an inner task name. Now only pops if outer name is not itself an inner task name. 2. The nested EnvGroup registration logic was duplicated across the train and eval dataset branches. Extracted into _register_nested_env_map so the collision check, registration, and conditional pop live in one place. 3. docs/reference.md EnvGroup entry was not updated to reflect the ValueError on unknown tasks or nested EnvGroup support. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed the new bugbot findings in commit 50dbd92:
|
mikasenghaas
left a comment
mikasenghaas
left a comment
There was a problem hiding this comment.
nice, i quite like this. we don't need this for prime-rl anymore since we moved away from EnvGroup for multi-env training but I still think supporting arbitrary nesting of EnvGroup might be nice. thoughts? @willccbb
2 participants
Summary
Two related fixes for
EnvGrouptask routing:1. Nested EnvGroup silently misroutes all tasks to envs[0]
When an
EnvGroupis wrapped inside anotherEnvGroup(as prime-rl does when registering user envs), the outer group was unconditionally overwriting the inner task names with a single outer name. The innerEnvGroupthen could not find those outer names in its ownenv_mapand silently fell back toenvs[0]for every rollout, and scored everything as 0.0.Fix: when a sub-env is itself an
EnvGroup, register each of its task names in the outerenv_mappointing to the inner group, and leave the inner task column in the dataset unchanged. Routing then flowsouter -> inner group -> correct sub-envas expected.2. Silent fallback masks misconfiguration
get_env_for_taskwas returningenvs[0]for any unrecognized task. This made it impossible to notice routing failures. It now raisesValueErrorwith the list of available task names.Test plan
test_get_env_for_taskupdated: unknown task raisesValueErrorinstead of returningenvs[0]test_nested_env_group_preserves_inner_tasks: wrapping an EnvGroup preserves inner task names in dataset and env_mapTestEnvGroupandTestEnvGroupRubrictests still passCloses #1008
Note
Medium Risk
Changes core rollout routing behavior in
EnvGroup(now raises on unknown tasks and alters nested task mapping), which could surface previously-hidden misconfigurations and break callers relying on fallback behavior.Overview
EnvGroup task routing is now strict and nesting-safe.
EnvGrouppreserves innertasklabels when anEnvGroupis wrapped inside anotherEnvGroup, expands the outerenv_mapto include the inner task names, and rejects task-name collisions at construction time.Misconfiguration is no longer silent.
get_env_for_tasknow raises aValueErrorlisting available tasks rather than defaulting toenvs[0]; tests were updated/added for unknown-task errors, nested routing, and name-collision handling, and docs were updated to describe the new behavior (includingenv_namesin the reference).Reviewed by Cursor Bugbot for commit 50dbd92. Bugbot is set up for automated code reviews on this repo. Configure here.